fix: keep dashboard interactive on TTYs#23
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds runtime-based CLI TUI mode detection and a CLI TUI context factory; updates the dashboard launcher to accept and normalize injected contexts and route between interactive TUI and non-interactive static output; and extends test timeouts and CLI-focused unit/integration tests. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as Client/CLI
participant Detector as Mode Detector
participant CtxFactory as Context Factory
participant Dashboard as Dashboard Launcher
participant TUI as Interactive TUI
participant Static as Static Lister
rect rgba(100,150,200,0.5)
Note over CLI, TUI: Interactive path
CLI->>Detector: detectCliTuiMode(runtime)
Detector-->>CLI: mode = 'interactive' / 'accessible'
CLI->>CtxFactory: createCliTuiContext(options)
CtxFactory-->>CLI: ctx (mode=interactive)
CLI->>Dashboard: launchDashboard(cas, {ctx, runApp})
Dashboard->>TUI: runApp(ctx)
TUI-->>Dashboard: renders interactive UI
end
rect rgba(200,150,100,0.5)
Note over CLI, Static: Non-interactive path
CLI->>Detector: detectCliTuiMode(runtime)
Detector-->>CLI: mode = 'pipe' / 'static'
CLI->>CtxFactory: createCliTuiContext(options)
CtxFactory-->>CLI: ctx (mode=pipe/static)
CLI->>Dashboard: launchDashboard(cas, {ctx, output})
Dashboard->>Static: printStaticList(cas, output)
Static->>Static: cas.listVault() -> format rows
Static-->>CLI: write rows to output stream
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/unit/cli/context.test.js (1)
15-40: Add explicit tests forBIJOU_ACCESSIBLEandTERM=dumbbranches.Current coverage is good, but these two branches are part of mode precedence and worth locking with direct tests to prevent regressions.
Suggested test additions
describe('detectCliTuiMode', () => { + it('uses accessible mode when BIJOU_ACCESSIBLE=1', () => { + const mode = detectCliTuiMode(makeRuntime({ + env: { BIJOU_ACCESSIBLE: '1', TERM: 'xterm-256color' }, + })); + expect(mode).toBe('accessible'); + }); + + it('falls back to pipe when TERM is dumb', () => { + const mode = detectCliTuiMode(makeRuntime({ + env: { TERM: 'dumb' }, + })); + expect(mode).toBe('pipe'); + }); + it('stays interactive on a TTY when NO_COLOR is set', () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/cli/context.test.js` around lines 15 - 40, Add two explicit unit tests for detectCliTuiMode: one that sets env BIJOU_ACCESSIBLE='1' (with TERM present) and asserts the returned mode is the accessible mode (use detectCliTuiMode with makeRuntime and expect mode === 'accessible' to lock that branch), and another that sets TERM='dumb' (with a TTY stdout) and asserts the returned mode is the static fallback (expect mode === 'static'); place them alongside the existing tests so the BIJOU_ACCESSIBLE and TERM=dumb branches are covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/ui/dashboard.js`:
- Around line 285-291: The JSDoc above printStaticList is incorrect—it's
describing an options object but the function signature is async function
printStaticList(cas, output = process.stdout). Update the JSDoc for
printStaticList to match the actual parameters by documenting the first
parameter (cas) and the second parameter (output, defaulting to process.stdout
and of type Pick<NodeJS.WriteStream,'write'> or a WriteStream), remove the
erroneous options block, and ensure types and brief descriptions reference the
exact symbol printStaticList and its parameters.
---
Nitpick comments:
In `@test/unit/cli/context.test.js`:
- Around line 15-40: Add two explicit unit tests for detectCliTuiMode: one that
sets env BIJOU_ACCESSIBLE='1' (with TERM present) and asserts the returned mode
is the accessible mode (use detectCliTuiMode with makeRuntime and expect mode
=== 'accessible' to lock that branch), and another that sets TERM='dumb' (with a
TTY stdout) and asserts the returned mode is the static fallback (expect mode
=== 'static'); place them alongside the existing tests so the BIJOU_ACCESSIBLE
and TERM=dumb branches are covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a0d224d0-e36b-4355-baf1-ba32305914fc
📒 Files selected for processing (14)
bin/ui/context.jsbin/ui/dashboard.jstest/integration/round-trip.test.jstest/integration/vault-cli.test.jstest/unit/cli/context.test.jstest/unit/cli/dashboard.launch.test.jstest/unit/cli/dashboard.test.jstest/unit/domain/services/CasService.compression.test.jstest/unit/domain/services/CasService.empty-file.test.jstest/unit/domain/services/CasService.envelope.test.jstest/unit/domain/services/CasService.kdf.test.jstest/unit/domain/services/rotateVaultPassphrase.test.jstest/unit/facade/ContentAddressableStore.rotation.test.jstest/unit/vault/VaultService.test.js
There was a problem hiding this comment.
🧹 Nitpick comments (1)
bin/ui/dashboard.js (1)
298-307: Tightenoptions.ctxcontract formodeto avoid silent fallback.Line 306 depends on
ctx.mode, but Line 299 documentsctxasBijouContext. If a caller passes a plainBijouContext(nomode), this path always goes static (undefined !== 'interactive'). Consider normalizing or explicitly typing the injected context shape.Suggested hardening
/** * `@param` {ContentAddressableStore} cas * `@param` {{ - * ctx?: BijouContext, + * ctx?: (BijouContext & { + * mode?: 'accessible' | 'pipe' | 'static' | 'interactive' + * }), * runApp?: typeof run, * output?: Pick<NodeJS.WriteStream, 'write'>, * }} [options] */ export async function launchDashboard(cas, options = {}) { const ctx = options.ctx || createCliTuiContext(); - if (ctx.mode !== 'interactive') { + const mode = ctx.mode ?? 'interactive'; + if (mode !== 'interactive') { return printStaticList(cas, options.output); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/ui/dashboard.js` around lines 298 - 307, The code relies on ctx.mode in launchDashboard but accepts a plain BijouContext which may lack mode; update launchDashboard to normalize/validate the injected context: if options.ctx is provided but options.ctx.mode is undefined, either set ctx = { ...options.ctx, mode: createCliTuiContext().mode } (or call createCliTuiContext() when mode is missing) or explicitly throw a clear error so callers must pass a context with mode. Locate the logic around launchDashboard, the options.ctx usage, createCliTuiContext, and the branch that calls printStaticList and ensure ctx.mode is always defined before comparing to 'interactive'.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@bin/ui/dashboard.js`:
- Around line 298-307: The code relies on ctx.mode in launchDashboard but
accepts a plain BijouContext which may lack mode; update launchDashboard to
normalize/validate the injected context: if options.ctx is provided but
options.ctx.mode is undefined, either set ctx = { ...options.ctx, mode:
createCliTuiContext().mode } (or call createCliTuiContext() when mode is
missing) or explicitly throw a clear error so callers must pass a context with
mode. Locate the logic around launchDashboard, the options.ctx usage,
createCliTuiContext, and the branch that calls printStaticList and ensure
ctx.mode is always defined before comparing to 'interactive'.
|
@coderabbitai Addressed in commits 3f8baf0 and e14eb84. Changes made:
One note on the earlier prompt: Review please 🥕 |
🧩 Analysis chain🏁 Script executed: Length of output: 5356 🏁 Script executed: Length of output: 1458
✅ What's solid
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
CHANGELOG.md (1)
24-24: Changelog description doesn't fully match the actual code behavior.The changelog states contexts without an explicit
modeare treated "as interactive," but the implementation callsdetectCliTuiMode()which returns one of four modes based on environment conditions:
'accessible'whenBIJOU_ACCESSIBLE=1'pipe'whenTERM=dumbor non-TTY'static'whenCIenv var is set'interactive'as the default fallbackConsider rewording to something like: "now properly detects the mode for injected Bijou contexts without an explicit
mode, avoiding an incorrect static fallback on real TTYs."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` at line 24, Update the changelog sentence to accurately reflect the behavior of launchDashboard(): it doesn't unconditionally treat contexts without mode as interactive but defers to detectCliTuiMode(), which can return 'accessible' (BIJOU_ACCESSIBLE=1), 'pipe' (TERM=dumb or non‑TTY), 'static' (CI set), or 'interactive' as the fallback; rephrase to something like "now properly detects the mode for injected Bijou contexts without an explicit mode (via detectCliTuiMode()), avoiding an incorrect static fallback on real TTYs" and mention the CLI tests lock BIJOU_ACCESSIBLE and TERM=dumb branches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@CHANGELOG.md`:
- Line 24: Update the changelog sentence to accurately reflect the behavior of
launchDashboard(): it doesn't unconditionally treat contexts without mode as
interactive but defers to detectCliTuiMode(), which can return 'accessible'
(BIJOU_ACCESSIBLE=1), 'pipe' (TERM=dumb or non‑TTY), 'static' (CI set), or
'interactive' as the fallback; rephrase to something like "now properly detects
the mode for injected Bijou contexts without an explicit mode (via
detectCliTuiMode()), avoiding an incorrect static fallback on real TTYs" and
mention the CLI tests lock BIJOU_ACCESSIBLE and TERM=dumb branches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3aa0138e-9a17-4cb1-8d2e-1088cf0038ac
📒 Files selected for processing (4)
CHANGELOG.mdbin/ui/dashboard.jstest/unit/cli/context.test.jstest/unit/cli/dashboard.launch.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- test/unit/cli/context.test.js
- test/unit/cli/dashboard.launch.test.js
There was a problem hiding this comment.
🧹 Nitpick comments (2)
bin/ui/dashboard.js (1)
295-310: Consider adding a defensive guard for undefinedruntime.If an injected context lacks both
modeandruntime, line 308 will calldetectCliTuiMode(undefined), which may throw or produce unexpected results. WhilecreateCliTuiContext()guarantees a runtime (per context snippet 1), manually injected contexts in tests or future code may not.This was flagged as a non-blocking concern in previous review comments. A minimal guard would make the contract explicit.
🛡️ Proposed defensive guard
function normalizeLaunchContext(ctx) { const candidate = /** `@type` {BijouContext & { mode?: import('@flyingrobots/bijou').OutputMode }} */ (ctx); if (candidate.mode) { return candidate; } + if (!candidate.runtime) { + throw new TypeError('ctx.runtime is required when ctx.mode is absent'); + } return { ...candidate, mode: detectCliTuiMode(candidate.runtime), }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/ui/dashboard.js` around lines 295 - 310, normalizeLaunchContext currently assumes ctx.runtime exists and calls detectCliTuiMode(candidate.runtime), which can misbehave if a context is injected without runtime; add a defensive guard in normalizeLaunchContext to supply a safe default runtime (or undefined-safe value) before calling detectCliTuiMode, e.g. check candidate.runtime and if missing provide the same default runtime used by createCliTuiContext or return candidate with a sensible mode (fallback) instead; update references in normalizeLaunchContext and the call to detectCliTuiMode so tests and manual injections don’t pass undefined.test/unit/cli/dashboard.launch.default.test.js (1)
6-12: Consider extendingmockCasto align with the actual CAS interface.The mock only stubs three methods. If future tests exercise additional CAS methods, they'll fail silently with
undefined. For a test helper, this is acceptable, but documenting the minimal interface or using a more complete stub could improve maintainability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/cli/dashboard.launch.default.test.js` around lines 6 - 12, The mockCas helper currently only implements listVault, getVaultMetadata, and readManifest which can cause future tests to fail if other CAS methods are called; update mockCas to implement the fuller CAS interface used across the codebase (at minimum add no-op/vi.fn().mockResolvedValue or mockRejectedValue stubs for commonly used methods such as writeObject, readObject, deleteObject, headObject, listObjects, and any metadata/update methods), keep the existing entries parameter behavior, and document in the mockCas JSDoc comment which methods are intentionally stubbed so callers know the minimal supported surface (refer to mockCas, listVault, getVaultMetadata, readManifest when locating where to add the extra stubs).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@bin/ui/dashboard.js`:
- Around line 295-310: normalizeLaunchContext currently assumes ctx.runtime
exists and calls detectCliTuiMode(candidate.runtime), which can misbehave if a
context is injected without runtime; add a defensive guard in
normalizeLaunchContext to supply a safe default runtime (or undefined-safe
value) before calling detectCliTuiMode, e.g. check candidate.runtime and if
missing provide the same default runtime used by createCliTuiContext or return
candidate with a sensible mode (fallback) instead; update references in
normalizeLaunchContext and the call to detectCliTuiMode so tests and manual
injections don’t pass undefined.
In `@test/unit/cli/dashboard.launch.default.test.js`:
- Around line 6-12: The mockCas helper currently only implements listVault,
getVaultMetadata, and readManifest which can cause future tests to fail if other
CAS methods are called; update mockCas to implement the fuller CAS interface
used across the codebase (at minimum add no-op/vi.fn().mockResolvedValue or
mockRejectedValue stubs for commonly used methods such as writeObject,
readObject, deleteObject, headObject, listObjects, and any metadata/update
methods), keep the existing entries parameter behavior, and document in the
mockCas JSDoc comment which methods are intentionally stubbed so callers know
the minimal supported surface (refer to mockCas, listVault, getVaultMetadata,
readManifest when locating where to add the extra stubs).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 98aea53a-fc5d-420f-9fd1-442cbd1785ca
📒 Files selected for processing (5)
bin/ui/dashboard.jstest/unit/cli/_testContext.jstest/unit/cli/context.test.jstest/unit/cli/dashboard.launch.default.test.jstest/unit/cli/dashboard.launch.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- test/unit/cli/context.test.js
- test/unit/cli/dashboard.launch.test.js
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/ui/dashboard.js`:
- Around line 85-89: The createInitModel function reads ctx.runtime.columns and
ctx.runtime.rows but normalizeLaunchContext only ensures runtime when mode is
missing, so add a defensive check in createInitModel: if ctx.runtime is falsy,
either assign a default runtime object (e.g., {columns: 80, rows: 24}) or throw
a clear error; update createInitModel (and/or normalizeLaunchContext) to
guarantee ctx.runtime exists before accessing properties so calls that pass a
context with mode set but no runtime don't cause a TypeError.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7e9be0b0-ed65-4957-91a3-750768f6f177
📒 Files selected for processing (2)
bin/ui/dashboard.jstest/unit/cli/dashboard.launch.test.js
✅ Files skipped from review due to trivial changes (1)
- test/unit/cli/dashboard.launch.test.js
| function createInitModel(ctx) { | ||
| return { | ||
| status: 'loading', | ||
| columns: process.stdout.columns ?? 80, | ||
| rows: process.stdout.rows ?? 24, | ||
| columns: ctx.runtime.columns ?? 80, | ||
| rows: ctx.runtime.rows ?? 24, |
There was a problem hiding this comment.
Potential TypeError if ctx has mode but no runtime.
normalizeLaunchContext only validates that runtime exists when mode is absent (lines 306-308). If a caller passes a context with mode set but runtime undefined, this function will throw TypeError: Cannot read properties of undefined (reading 'columns').
Consider adding a defensive check or documenting the invariant that ctx.runtime is always required.
Suggested defensive check
function createInitModel(ctx) {
+ const runtime = ctx.runtime ?? {};
return {
status: 'loading',
- columns: ctx.runtime.columns ?? 80,
- rows: ctx.runtime.rows ?? 24,
+ columns: runtime.columns ?? 80,
+ rows: runtime.rows ?? 24,
entries: [],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/ui/dashboard.js` around lines 85 - 89, The createInitModel function reads
ctx.runtime.columns and ctx.runtime.rows but normalizeLaunchContext only ensures
runtime when mode is missing, so add a defensive check in createInitModel: if
ctx.runtime is falsy, either assign a default runtime object (e.g., {columns:
80, rows: 24}) or throw a clear error; update createInitModel (and/or
normalizeLaunchContext) to guarantee ctx.runtime exists before accessing
properties so calls that pass a context with mode set but no runtime don't cause
a TypeError.
|
@coderabbitai Fresh review requested. A carrot is promised for your troubles 🥕 |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/unit/cli/dashboard.launch.test.js (1)
38-53: Test implicitly depends oncreateTestContextruntime handling.The test passes
envas an object (line 42), butdetectCliTuiModecallsruntime.env()as a function. This works becausecreateTestContextpresumably wraps the env object. If the test helper's behavior changes, this test could break silently.Consider making the runtime explicit to decouple from helper internals:
Suggested explicit runtime
it('treats an injected context without mode as interactive', async () => { const cas = mockCas(); + const envMap = { TERM: 'xterm-256color' }; const ctx = { ...makeCtx('interactive', { - env: { TERM: 'xterm-256color' }, + env: (key) => envMap[key], stdoutIsTTY: true, stdinIsTTY: true, }), };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/cli/dashboard.launch.test.js` around lines 38 - 53, The test relies on createTestContext's wrapping of env as a function, so update the injected context passed to launchDashboard to provide the runtime shape detectCliTuiMode expects: replace the plain env object with a runtime-like API (e.g., provide env as a function or a runtime object with env() returning the env), and ensure stdoutIsTTY/stdinIsTTY are set on that runtime or on the ctx the code reads; modify the call site created by makeCtx/ctx so detectCliTuiMode sees runtime.env() explicitly, avoiding implicit dependence on createTestContext internals when calling launchDashboard and makeCtx.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/unit/cli/dashboard.launch.test.js`:
- Around line 38-53: The test relies on createTestContext's wrapping of env as a
function, so update the injected context passed to launchDashboard to provide
the runtime shape detectCliTuiMode expects: replace the plain env object with a
runtime-like API (e.g., provide env as a function or a runtime object with env()
returning the env), and ensure stdoutIsTTY/stdinIsTTY are set on that runtime or
on the ctx the code reads; modify the call site created by makeCtx/ctx so
detectCliTuiMode sees runtime.env() explicitly, avoiding implicit dependence on
createTestContext internals when calling launchDashboard and makeCtx.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9660cfdd-34c0-4e43-b5a0-e62df31f71a2
📒 Files selected for processing (2)
bin/ui/dashboard.jstest/unit/cli/dashboard.launch.test.js
Summary
NO_COLOR=1launchDashboard()fall back to the static list for non-interactive CLI contexts instead of rendering a one-frame TUITesting
npx eslint .npm testGIT_STUNTS_DOCKER=1 npx vitest run test/integrationbunx vitest run test/unitGIT_STUNTS_DOCKER=1 bunx vitest run test/integrationdeno run -A npm:vitest run test/unitGIT_STUNTS_DOCKER=1 deno run -A npm:vitest run test/integrationNotes
NO_COLOR=1, the Bijou context downgraded topipemode and the dashboard rendered once before exiting.Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation